943 seo data optimization error reporting refactoring#23402
Conversation
Coverage Report for CI Build 9Coverage increased (+0.1%) to 55.441%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats💛 - Coveralls |
Added the getAlreadyIndexedStateName function because the Tools page and the FTC use a different string. All the rest is 100% untouched
Nothing changed, I just moved out the re-used code
… id and type to be consumed by the fronted
…rminal which indexable cannot be created
600e5a2 to
3ecba73
Compare
|
A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch. |
There was a problem hiding this comment.
Pull request overview
This PR improves SEO data optimization/indexing failure reporting by propagating the failing object’s identity (type + ID) from the PHP indexable build layer through to WP-CLI output and REST/React error UI, and refactors the JS indexation engine into a shared base component.
Changes:
- Introduces
Indexing_Failed_Exceptionto wrap unexpected build failures while carrying failing object metadata, and emits a newwpseo_indexable_indexing_failedaction. - Extends REST (
Indexing_Route) and WP-CLI (Index_Command) error handling to surface the failing object (type + ID) in responses/output. - Refactors JS indexation into
AbstractIndexationand centralizes error-details rendering inIndexingErrorContent, with updated unit/Jest coverage.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/Routes/Indexing_Route_Test.php | Adds unit coverage for turning Indexing_Failed_Exception into a REST WP_Error. |
| tests/Unit/Exceptions/Indexable/Indexing_Failed_Exception_Test.php | Adds unit coverage for the new exception’s stored metadata and message. |
| tests/Unit/Commands/Index_Command_Test.php | Adds unit coverage for WP-CLI output including failing object details. |
| tests/Unit/Builders/Indexable_Builder/Save_Indexable_Test.php | Updates builder tests for the new constructor dependency (logger) and improves phpdoc typing. |
| tests/Unit/Builders/Indexable_Builder/Maybe_Build_Author_Indexable_Test.php | Updates builder tests for the new constructor dependency (logger). |
| tests/Unit/Builders/Indexable_Builder/Is_Type_With_No_Id_Test.php | Updates builder tests for the new constructor dependency (logger) and corrects provider docblock. |
| tests/Unit/Builders/Indexable_Builder/Ensure_Indexable_Test.php | Updates builder tests for the new constructor dependency (logger). |
| tests/Unit/Builders/Indexable_Builder/Build_Test.php | Adds coverage for logging + action firing + wrapped exception rethrow on unexpected build errors. |
| tests/Unit/Builders/Indexable_Builder/Abstract_Indexable_Builder_TestCase.php | Adds a logger mock to the shared builder test setup; improves phpdoc typing. |
| src/routes/indexing-route.php | Converts Indexing_Failed_Exception into WP_Error with stack trace and failing object metadata. |
| src/exceptions/indexable/indexing-failed-exception.php | Adds the new exception class to carry failing object identity and wrap the underlying throwable. |
| src/commands/index-command.php | Enhances WP-CLI indexing to halt with a clearer error including failing object info. |
| src/builders/indexable-builder.php | Logs unexpected build failures, fires wpseo_indexable_indexing_failed, and rethrows as Indexing_Failed_Exception. |
| packages/js/tests/indexation.test.js | Adds Jest coverage for showing the failing object in the error details UI. |
| packages/js/src/first-time-configuration/tailwind-components/steps/indexation/indexing-error.js | Reuses shared IndexingErrorContent for consistent error-details rendering. |
| packages/js/src/first-time-configuration/tailwind-components/steps/indexation/indexation.js | Refactors FTC indexation UI to extend shared AbstractIndexation. |
| packages/js/src/errors/RequestError.js | Extends RequestError to carry failing object metadata. |
| packages/js/src/components/IndexingErrorContent.js | Introduces shared error-details renderer, including a “Failing object” line. |
| packages/js/src/components/IndexingError.js | Refactors to use shared IndexingErrorContent. |
| packages/js/src/components/Indexation.js | Refactors Tools-page indexation UI to extend AbstractIndexation. |
| packages/js/src/components/AbstractIndexation.js | Introduces shared indexation request/state engine and shared propTypes/defaultProps. |
| composer.json | Updates YoastCS error threshold value. |
| /** | ||
| * Tests that a failing object reported through an Indexing_Failed_Exception is turned into a | ||
| * WP_Error carrying that object's id and type. | ||
| * | ||
| * @covers ::run_indexation_action | ||
| * | ||
| * @return void | ||
| */ | ||
| public function test_index_posts_when_indexing_failed_exception_occurs() { | ||
| $exception = new Indexing_Failed_Exception( 123, 'post', 'post', new Exception( 'The underlying error.' ) ); | ||
|
|
||
| $this->post_indexation_action->expects( 'index' )->once()->andThrow( $exception ); | ||
|
|
||
| $this->indexing_helper->expects( 'indexing_failed' )->once()->withNoArgs(); | ||
|
|
||
| Mockery::mock( WP_Error::class ); | ||
|
|
||
| $this->assertInstanceOf( WP_Error::class, $this->instance->index_posts() ); | ||
| } |
There was a problem hiding this comment.
I'll leave this to @pls78 whether he wants to deal with that or not
There was a problem hiding this comment.
I agree with the changing the test to cover also the object id, but the proposed approach fails because overload requires the class to be undeclared at mock-creation time — and WP_Error never is. overload: works by Mockery declaring a class with that exact name so it can intercept new WP_Error(...). If the class already exists in the process, Mockery throws ("could not load mock… class already exists"). The whole unit suite runs in one PHP process, and WP_Error is the most-mocked WP native in it — remember the final incident: ~88 tests across unrelated suites do Mockery::mock( WP_Error::class ), and mocking an undefined class also declares it. So whichever of those ~88 tests runs first poisons WP_Error for overloading in every later test. .
I have added a note in the relevant tech choices about the test implementation.
| } | ||
|
|
||
| Indexation.propTypes = { | ||
| indexingActions: PropTypes.object, | ||
| preIndexingActions: PropTypes.object, | ||
| indexingStateCallback: PropTypes.func, | ||
| }; | ||
|
|
||
| Indexation.defaultProps = { | ||
| indexingActions: {}, | ||
| preIndexingActions: {}, | ||
| indexingStateCallback: () => {}, | ||
| }; | ||
|
|
||
| export default Indexation; |
There was a problem hiding this comment.
I'll leave this to @pls78 whether he wants to deal with that or not
There was a problem hiding this comment.
I guess this wouldn't hurt :)
leonidasmi
left a comment
There was a problem hiding this comment.
CR: 🏗️
A couple of remarks aside from the inline ones:
object_idcan be null for id-less types. Forhome-page,date-archive,post-type-archive, andsystem-page indexablesobject_id is null,- so the exception message renders "for object 0" and the CLI prints "Could not optimize system-page #0 while indexing general objects". I think we should improve that, for better UX (maybe the message could fall back to object_sub_type for those types?)
- it also means that we should update the
$object_iddocblocks (in exception constructor and the action doc) to beint|null
- There are cases where building an indexable means building an additional indexable during the initial build (eg. when building a post indexable, we also do
$this->maybe_build_author_indexable( $indexable->author_id ).- Which means that if there's something wrong with building the second/inner indexable, we will initiate the improvements of this PR twice. Specifically:
- a second log entry, a second action fire (now with the post's identity and the already-wrapped exception as $exception), and a message like "could not build the post indexable for object 5: Yoast SEO could not build the user indexable for object 3: …".
- There's an easy fix where we can add a
catch ( Indexing_Failed_Exception $exception )beforecatch ( Throwable $exception ), in the builder - if we decide to do that, let's also add test instructions for that case
- Which means that if there's something wrong with building the second/inner indexable, we will initiate the improvements of this PR twice. Specifically:
Also, let's add test instructions for the Logger addition, since support might use this PR for future reference and I suspect that they are gonna use our Logger for troubleshooting :)
|
A merge conflict has been detected for the proposed code changes in this PR. Please resolve the conflict by either rebasing the PR or merging in changes from the base branch. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Context
When an unexpected error occurred while building an indexable during SEO data optimization, the failure surfaced as an opaque error: the user had no indication of which object could not be optimized. This PR carries the failing object's identity (type + ID) from the indexable builder all the way through to the WP-CLI output and the REST/React error UI, and gives third parties a hook to react to the failure.
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
Indexing_Failed_Exception(extendingIndexable_Exception) wraps the underlying throwable and carries the object'sobject_id,object_type, andobject_sub_type.wpseo_indexable_indexing_failedaction so third parties can react to a failed indexable build.AbstractIndexationcomponent so the Tools page and the first-time configuration share a single code path, with the render layer kept in thin subclasses.WP_Error_Double(namespaced, intests/Unit/Doubles/) to the globalWP_Errorname viaclass_alias, so the route test can assert on the actual code, message and data payload of the returned error. The existingmakeDoublesForUnavailableClasses()fakes are method-less property bags that only supportinstanceOfassertions, and declaring a literal globalWP_Errorclass file would have required three PHPCS exemptions in.phpcs.xml.dist. The double is deliberately notfinal, since existing tests mockWP_Errorthrough Mockery.Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR changes what the user sees when an indexable cannot be built during SEO data optimization. To test it you need to (a) make sure indexing actually runs, (b) force a build failure, then (c) check the CLI output and the React error UI. Finally, confirm the normal (happy-path) optimization still works once the forced failure is removed.
Setup
Force a build failure (temporary code change)
src/builders/indexable-post-builder.phpand, as the very first line ofpublic function build( $post_id, $indexable )(around line 115), add:Indexable_Builder::build()catches, logs, fireswpseo_indexable_indexing_failedfor, and re-throws asIndexing_Failed_Exception— exactly the path this PR adds, with object typepostand a real post ID.Check the React error UI (Tools page)
post #{id}(the ID of the post that failed). The Request URL, Request method, Status code, Error message, and a collapsible "Error stack trace" should also be present.Check the WP-CLI output
wp yoast index(in this worktree-based Docker setup, run it inside the cli container or viawp-env run cli wp yoast index).Could not optimize post #{id} while indexing posts: Simulated indexing failureand stops with an error, instead of an opaque failure.Check the case where an indexable creation triggers another indexable creation
Tools->Yoast Testwp yoast indexError: Could not optimize user #<author ID> while indexing posts: QA: forced author build failureYoast SEO->Tools->SEO data optimizationand clickStart SEO data optimizationYoast SEO could not build the indexable for user #<ID>: QA: forced author build failure.Error details, the Failing object line readsuser #<ID>(Optional) Third-party hook + log
WP_DEBUG_LOGenabled, the builder's own logger also writes the failure todebug.log.Confirm the happy path still works (no regression)
throw(and the optional hook).AbstractIndexation) as the Tools page.Relevant test scenarios
{object_type} #{object_id}for whichever indexable fails — to see a non-post type, move the temporarythrowinto the matching builder (e.g.indexable-term-builder.phpforterm #{id}).Test instructions for QA when the code is in the RC
Forcing a build failure requires a temporary code change, so QA can focus on the happy path plus a visual check of the error UI:
wp yoast indexWP-CLI command still indexes content normally and reports completion.throwinindexable-post-builder.php), confirm the Tools-page error alert's "Error details" shows a "Failing object" line aspost #{id}, and thatwp yoast indexprintsCould not optimize post #{id} while indexing posts: …. Remove the temporary change afterward.Impact check
This PR affects the following parts of the plugin, which may require extra testing:
Indexable_Builder) — now logs, fireswpseo_indexable_indexing_failed, and re-throws asIndexing_Failed_Exceptionon unexpected errors.wp yoast indexWP-CLI command and the indexing REST route — both now handle the new exception.AbstractIndexationengine, so the happy-path indexing flow should be regression-tested in both places.Other environments
[shopify-seo], added test instructions for Shopify and attached theShopifylabel to this PR.[yoast-doc-extension], added test instructions for Yoast SEO for Google Docs and attached theGoogle Docs Add-onlabel to this PR.Documentation
Quality assurance
grunt build:imagesand committed the results, if my PR introduces or edits images or SVGs.Innovation
innovationlabel.Fixes #943